feat: Preference option for configuring eim_idf.json file#1444
Conversation
📝 WalkthroughWalkthroughCentralizes eim_idf.json path resolution in EimIdfJsonPathResolver, exposes a preference and UI to set a custom path, updates consumers to use the resolver, and refactors the watch service for restartability and null-safe lifecycle handling. ChangesCore Path Resolution Infrastructure
EIM Watch Service & Lifecycle
Consumers & UI Preferences
|Preferences page and localization Sequence DiagramsequenceDiagram
participant User
participant PreferencesUI as Preferences Page
participant PrefStore as Preference Store
participant Resolver as EimIdfJsonPathResolver
participant WatchService as EimJsonWatchService
participant FS as Filesystem
User->>PreferencesUI: set custom eim_idf.json path
PreferencesUI->>PrefStore: save EIM_IDF_JSON_PATH
PreferencesUI->>WatchService: restartAfterEimIdfPathChange()
WatchService->>Resolver: resolveEimIdfJsonFile()
Resolver->>PrefStore: read EIM_IDF_JSON_PATH
Resolver->>FS: validate custom path (basename & regular file)
Resolver-->>WatchService: resolved Path (custom or default)
WatchService->>WatchService: create new instance & reattach listeners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java (1)
138-174:⚠️ Potential issue | 🟠 MajorKeep pause/unpause balanced on the same watcher instance.
The code pauses one
getInstance()result and later unpauses another. If the watcher is restarted/recreated while EIM is open, or ifcallback.run()throws, listeners can remain paused on the original watcher.Suggested fix
waitJob.addJobChangeListener(new JobChangeAdapter() { + private EimJsonWatchService pausedWatchService; + `@Override` public void aboutToRun(IJobChangeEvent event) { - EimJsonWatchService ws = EimJsonWatchService.getInstance(); - if (ws != null) + pausedWatchService = EimJsonWatchService.getInstance(); + if (pausedWatchService != null) { - ws.pauseListeners(); + pausedWatchService.pauseListeners(); } } @@ public void done(IJobChangeEvent event) { - Display.getDefault().asyncExec(() -> { - try - { - standardConsoleStream.write("EIM has been closed.\n"); //$NON-NLS-1$ - } - catch (IOException e) - { - Logger.log(e); - } - }); - - if (callback != null) + try { - callback.run(); + Display.getDefault().asyncExec(() -> { + try + { + standardConsoleStream.write("EIM has been closed.\n"); //$NON-NLS-1$ + } + catch (IOException e) + { + Logger.log(e); + } + }); + + if (callback != null) + { + callback.run(); + } } - - EimJsonWatchService ws2 = EimJsonWatchService.getInstance(); - if (ws2 != null) + finally { - ws2.unpauseListeners(); + if (pausedWatchService != null) + { + pausedWatchService.unpauseListeners(); + pausedWatchService = null; + } } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java` around lines 138 - 174, The aboutToRun/done listener currently pauses EimJsonWatchService.getInstance() into local ws and later calls getInstance() again (ws2) and runs callback, risking an unmatched unpause if the watcher is recreated or callback throws; capture the watcher instance once (e.g., a final variable like pausedWatcher) when aboutToRun runs or before adding the JobChangeAdapter and use that same reference in done, and ensure done wraps callback.run() in try/finally so pausedWatcher.unpauseListeners() is always invoked on the same instance even if callback throws.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
23-64:⚠️ Potential issue | 🟠 MajorTrack the resolved path with the stored timestamp.
Line 64 makes the watched file configurable, but
LAST_MODIFIED_PREF_KEYstill stores a timestamp without the path it belongs to. After switching locations, a newly selectedeim_idf.jsoncan be treated as unchanged if its mtime is older than the previous file’s timestamp.Suggested fix
private static final String LAST_MODIFIED_PREF_KEY = "lastEimJsonModified"; //$NON-NLS-1$ + private static final String LAST_PATH_PREF_KEY = "lastEimJsonPath"; //$NON-NLS-1$ @@ public boolean wasModifiedSinceLastRun() { - File jsonFile = new File(getEimJsonPath()); + String eimJsonPath = getEimJsonPath(); + File jsonFile = new File(eimJsonPath); if (!jsonFile.exists()) { return false; } long lastModified = jsonFile.lastModified(); long lastSeen = preferences.getLong(LAST_MODIFIED_PREF_KEY, 0L); @@ return false; } + String lastPath = preferences.get(LAST_PATH_PREF_KEY, eimJsonPath); + if (!eimJsonPath.equals(lastPath)) + { + return true; + } + return lastModified > lastSeen; } @@ public void updateLastSeenTimestamp() { - File jsonFile = new File(getEimJsonPath()); + String eimJsonPath = getEimJsonPath(); + File jsonFile = new File(eimJsonPath); if (jsonFile.exists()) { preferences.putLong(LAST_MODIFIED_PREF_KEY, jsonFile.lastModified()); + preferences.put(LAST_PATH_PREF_KEY, eimJsonPath); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java` around lines 23 - 64, The stored timestamp under LAST_MODIFIED_PREF_KEY in EimJsonStateChecker doesn't account for the resolved file path from getEimJsonPath(), so switching to a different eim_idf.json can be misdetected; update wasModifiedSinceLastRun and updateLastSeenTimestamp to also persist and compare the resolved path (e.g., store a companion preference like lastEimJsonPath or include the path in the pref key) before using the stored timestamp—if the saved path differs from the current getEimJsonPath() result treat it as a first-seen file (or reset the timestamp) and then save the current path together with the file's lastModified value so future calls to wasModifiedSinceLastRun check both path equality and mtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfJsonPathResolver.java`:
- Around line 30-40: The resolver currently returns any existing path; update
resolveEimIdfJsonFileFromPreferenceString to validate that the provided custom
path has the expected basename (compare
Paths.get(custom).getFileName().toString() to EimConstants.EIM_JSON) and that it
is a regular file (use Files.isRegularFile) before returning it; otherwise fall
back to getDefaultEimIdfJsonFile(). Keep the existing null/empty check and use
the same Path p variable and Files.exists check logic but replace the
exists-only check with the basename + isRegularFile guards so the watcher/UI
contract is honored.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 63-66: The current isEimIdfJsonPresent() uses Files.exists(...)
which can return true for directories or non-regular paths; change the check to
verify the resolved path from new
EimIdfJsonPathResolver().resolveEimIdfJsonFile() is a regular readable file (use
Files.isRegularFile(path) and/or Files.isReadable(path) instead of Files.exists)
so the method only returns true for an actual file that can be read.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java`:
- Around line 92-114: restartAfterEimIdfPathChange currently stops the old
watcher first which can leave INSTANCE null if constructing the new
EimJsonWatchService fails; change the order in restartAfterEimIdfPathChange so
you first create the new EimJsonWatchService instance (new
EimJsonWatchService()), register the copied listeners on that fresh instance via
fresh.addEimJsonChangeListener(...), then atomically assign INSTANCE = fresh,
and only after the swap call old.requestStop(); keep the try/catch around the
construction so if construction throws you log the IOException and leave the
original INSTANCE running (do not set INSTANCE to null).
In
`@tests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java`:
- Around line 50-55: The test nonexistent_custom_falls_back_to_default assumes a
literal filesystem path that might exist on the host; make it deterministic by
using JUnit's `@TempDir`. Change the test to accept a `@TempDir` Path (e.g. Path
tempDir) and pass a guaranteed-missing child like
tempDir.resolve("missing").resolve("eim_idf.json").toString() into
EimIdfJsonPathResolver.resolveEimIdfJsonFileFromPreferenceString, keeping the
assertion against r.getDefaultEimIdfJsonFile(); reference the test method name
and the EimIdfJsonPathResolver class to locate where to update the parameter and
call.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java`:
- Around line 138-174: The aboutToRun/done listener currently pauses
EimJsonWatchService.getInstance() into local ws and later calls getInstance()
again (ws2) and runs callback, risking an unmatched unpause if the watcher is
recreated or callback throws; capture the watcher instance once (e.g., a final
variable like pausedWatcher) when aboutToRun runs or before adding the
JobChangeAdapter and use that same reference in done, and ensure done wraps
callback.run() in try/finally so pausedWatcher.unpauseListeners() is always
invoked on the same instance even if callback throws.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java`:
- Around line 23-64: The stored timestamp under LAST_MODIFIED_PREF_KEY in
EimJsonStateChecker doesn't account for the resolved file path from
getEimJsonPath(), so switching to a different eim_idf.json can be misdetected;
update wasModifiedSinceLastRun and updateLastSeenTimestamp to also persist and
compare the resolved path (e.g., store a companion preference like
lastEimJsonPath or include the path in the pref key) before using the stored
timestamp—if the saved path differs from the current getEimJsonPath() result
treat it as a first-seen file (or reset the timestamp) and then save the current
path together with the file's lastModified value so future calls to
wasModifiedSinceLastRun check both path equality and mtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c910487-d1c4-4aae-9ef8-a3cb4e454c42
📒 Files selected for processing (14)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfJsonPathResolver.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.javabundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages_zh.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM. Just a one small nit about small UX improvement
| eimGroup.setLayout(new GridLayout(3, false)); | ||
|
|
||
| Label eimPathLabel = new Label(eimGroup, SWT.None); | ||
| eimPathLabel.setText(Messages.EspresssifPreferencesPage_EimIdfJsonPathLabel); |
9c43397 to
dbf78f1
Compare
|
@kolipakakondal hi ! Tested under: Windows 11: Mac arm64: |
dbf78f1 to
a506780
Compare
Hi Andrew, the issue you are referring to should already be fixed on master. I rebased this branch onto master. Please take another look. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java`:
- Around line 62-64: The checker currently uses getEimJsonPath() to resolve a
user-configurable path but stores a single lastEimJsonModified value that can be
compared across different files; change the state to key the last-seen timestamp
by the resolved path (e.g., replace or augment the single lastEimJsonModified
field with a Map<String,Long> like lastEimJsonModifiedByPath) and update uses in
EimJsonStateChecker methods (those that call getEimJsonPath(), compare
modification times, and update the stored value) to read/update the timestamp
for the specific resolved path instead of a global timestamp so preference
changes won’t mix files’ timestamps.
In `@docs/zh_CN/installation.rst`:
- Line 81: Replace the escaped Windows path literal
"C:\\Espressif\\tools\\eim_idf.json" in the docs/zh_CN/installation.rst text
with a normal Windows path literal using single backslashes (e.g.
"C:\Espressif\tools\eim_idf.json") so the rendered/copied path isn't shown with
doubled separators; update the exact string in the sentence that mentions EIM
保存路径.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23825f0c-3bf5-4402-a7c8-991ba752724b
📒 Files selected for processing (16)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfJsonPathResolver.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.javabundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages_zh.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javadocs/en/installation.rstdocs/zh_CN/installation.rsttests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java
✅ Files skipped from review due to trivial changes (3)
- docs/en/installation.rst
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages_zh.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
🚧 Files skipped from review as they are similar to previous changes (10)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfJsonPathResolver.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java
| private String getEimJsonPath() | ||
| { | ||
| return Platform.getOS().equals(Platform.OS_WIN32) ? EimConstants.EIM_WIN_PATH : EimConstants.EIM_POSIX_PATH; | ||
| return new EimIdfJsonPathResolver().resolveEimIdfJsonFile().toString(); |
There was a problem hiding this comment.
Bind last-seen timestamp to the resolved file path.
Because Line 64 now resolves a user-configurable path, keeping a single lastEimJsonModified value can compare different files across preference changes, causing stale modification checks.
🛠️ Suggested fix
private static final String LAST_MODIFIED_PREF_KEY = "lastEimJsonModified"; //$NON-NLS-1$
+private static final String LAST_PATH_PREF_KEY = "lastEimJsonPath"; //$NON-NLS-1$
@@
public boolean wasModifiedSinceLastRun()
{
- File jsonFile = new File(getEimJsonPath());
+ String currentPath = getEimJsonPath();
+ File jsonFile = new File(currentPath);
if (!jsonFile.exists())
{
return false;
}
+ String lastPath = preferences.get(LAST_PATH_PREF_KEY, ""); //$NON-NLS-1$
+ if (!currentPath.equals(lastPath))
+ {
+ return false;
+ }
@@
public void updateLastSeenTimestamp()
{
- File jsonFile = new File(getEimJsonPath());
+ String currentPath = getEimJsonPath();
+ File jsonFile = new File(currentPath);
if (jsonFile.exists())
{
+ preferences.put(LAST_PATH_PREF_KEY, currentPath);
preferences.putLong(LAST_MODIFIED_PREF_KEY, jsonFile.lastModified());
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java`
around lines 62 - 64, The checker currently uses getEimJsonPath() to resolve a
user-configurable path but stores a single lastEimJsonModified value that can be
compared across different files; change the state to key the last-seen timestamp
by the resolved path (e.g., replace or augment the single lastEimJsonModified
field with a Map<String,Long> like lastEimJsonModifiedByPath) and update uses in
EimJsonStateChecker methods (those that call getEimJsonPath(), compare
modification times, and update the stored value) to read/update the timestamp
for the specific resolved path instead of a global timestamp so preference
changes won’t mix files’ timestamps.
| ``eim_idf.json`` 路径 | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| EIM 用 ``eim_idf.json`` 保存你的 ESP-IDF 安装信息。默认路径为:**Windows** — ``C:\\Espressif\\tools\\eim_idf.json``;**Linux / macOS** — ``~/.espressif/tools/eim_idf.json``。 |
There was a problem hiding this comment.
Use canonical Windows path formatting in docs.
Line 81 currently documents C:\\Espressif\\tools\\eim_idf.json; this will render/copy as doubled separators. Use a normal Windows path literal to avoid user confusion.
✍️ Proposed doc fix
-EIM 用 ``eim_idf.json`` 保存你的 ESP-IDF 安装信息。默认路径为:**Windows** — ``C:\\Espressif\\tools\\eim_idf.json``;**Linux / macOS** — ``~/.espressif/tools/eim_idf.json``。
+EIM 用 ``eim_idf.json`` 保存你的 ESP-IDF 安装信息。默认路径为:**Windows** — ``C:\Espressif\tools\eim_idf.json``;**Linux / macOS** — ``~/.espressif/tools/eim_idf.json``。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/zh_CN/installation.rst` at line 81, Replace the escaped Windows path
literal "C:\\Espressif\\tools\\eim_idf.json" in the docs/zh_CN/installation.rst
text with a normal Windows path literal using single backslashes (e.g.
"C:\Espressif\tools\eim_idf.json") so the rendered/copied path isn't shown with
doubled separators; update the exact string in the sentence that mentions EIM
保存路径.
@sigmaaa hi LGTM 👍 |

Description
Preference option for configuring eim_idf.json file from the custom location.
Fixes # IEP-1753
Type of change
New feature (non-breaking change which adds functionality)
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation